[rc2] Fix transformation from JsonQueryExpression to OPENJSON#36642
[rc2] Fix transformation from JsonQueryExpression to OPENJSON#36642roji merged 2 commits intodotnet:release/10.0from
Conversation
|
|
||
| break; | ||
| var containerColumnName = structuralType.GetContainerColumnName(); | ||
| var containerColumnCandidates = structuralType.ContainingEntityType.GetTableMappings() |
There was a problem hiding this comment.
@AndriySvyryd this is a hack to get the container column (IColumn) containing a given structural type (needed here in order to get its type mapping nvarchar(max) or json).
We can easily get the name (as we do just above). I assumed that GetContainerColumnType() would give me the store type, but that seems to work only when the user explicitly configures the JSON column type via HasColumnType() (otherwise it's null). So I sort of hacked it here, simply getting all columns mapped to the containing entity type, and than matching it by name. Note that in theory an entity can have two columns with the same name (in different tables via entity splitting), so this simple by-name matching doesn't work there and will throw (though this is obviously a bit contrived).
I'm assuming this is related to the lack of a relational model representation of JSON and/or JSON columns. Let me know if I've missed a better way to do this or should reference an issue tracking making this kind of thing better.
There was a problem hiding this comment.
Just one thought - should GetContainerColumnType() always return the correct type even if the user hasn't set it explicitly? That would allow me to just call it here and be done with it (regardless of the bigger relational JSON modeling).
There was a problem hiding this comment.
#36647, but I don't think it fits in 10, it's more involved than how it sounds
There was a problem hiding this comment.
OK, we'll keep it as it is, no problem.
AndriySvyryd
changed the title
| { | ||
| [var c] => c, | ||
| [] => throw new UnreachableException($"No container column found in relational model for {structuralType.DisplayName()}"), | ||
| _ => throw new InvalidOperationException( |
There was a problem hiding this comment.
This should be UnreachableException too. We validate against it
There was a problem hiding this comment.
Will change. But to be sure I understand, you mean that we don't allow mapping two properties on the same entity to different columns in different tables (entity splitting) if those columns happen to have the same name (again, on two different tables)? Shouldn't we support that?
That problem here is specifically that because of the lack of proper metadata, we're forced to just lookup by column name (from GetContainerColumnName()) and have no additional context on which table that column is on, etc.
There was a problem hiding this comment.
We should allow it, but we don't support that yet. And we need the Relational Model support to allow this in a sensible manner.
3 participants
Fixes #36628
Description
When transforming JsonQueryExpression to SqlServerOpenJsonExpression, nested JSON objects were always projected out of OPENJSON as
nvarchar(max), even if the containing column uses the new JSON data type; this caused subsequent JSON_VALUE() operations to fail, since the RETURNING clause which gets generated only works with the new JSON data type (regardless of this bug, nested JSON objects should always have the same type as the container JSON column).This bug was missed since we don't test against SQL Server 2025 in CI (where the JSON data type can be exercised), only against SQL Server 2022.
Customer impact
Various queries fail which involve LINQ operators on top of JSON collections (major EF 10 feature), where the collection element itself has nested JSON objects.
How found
Testing by @AndriySvyryd in #36628.
Regression
No
Testing
Tests were already there, but are not currently being executed against SQL Server 2025 in CI.
Risk
Low, the fix is very targeted and affects only this specific scenario.